fix Auto import suggestions for collections.abc should take precedence over typing #1685 #1949

asukaminato0721 · 2025-12-29T08:10:45Z

Summary

Addressed issue 1685 by making autoimport prefer public shim modules (like collections.abc) over their private implementations and ensuring private entries are deprioritized when a public alternative exists.

Updated to recognize dotted-name private shims (e.g., _collections_abc -> collections.abc) and to sort autoimport candidates so private imports fall behind public ones when both exist.

Test Plan

test autoimport_prefers_public_reexport_for_dotted_private_module

Copilot

Pull request overview

This PR addresses issue #1685 by making auto-import suggestions prefer public module re-exports (like collections.abc) over their private implementation modules (like _collections_abc). The fix implements two key changes: (1) extending the should_include_reexport function to recognize dotted-name private shims that encode dots as underscores, and (2) deprioritizing private imports in completion sorting when a public alternative exists.

Key Changes:

Enhanced private module shimming detection to handle stdlib patterns like _collections_abc → collections.abc
Modified autoimport completion logic to track and deprioritize private imports when public alternatives exist
Updated sort_text assignment to ensure consistent lexicographic ordering of completion items

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`pyrefly/lib/state/lsp.rs`	Implements two-pass autoimport candidate collection: first pass collects all candidates and tracks which names have public exports, second pass assigns lower priority sort_text to private imports with public alternatives. Also extends `should_include_reexport` to handle dotted module name patterns.
`pyrefly/lib/test/lsp/completion.rs`	Adds test validating that public re-exports (`foo.bar`) are suggested before their private implementations (`_foo_bar`) in autoimport completions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pyrefly/lib/state/lsp.rs

Copilot · 2025-12-29T08:15:10Z

pyrefly/lib/state/lsp.rs

+        // Some stdlib shims encode dotted modules with underscores (e.g. _collections_abc).
+        if canonical_module.as_str().starts_with('_') && original_module.as_str().contains('.') {
+            let canonical_trim = canonical_module.as_str().trim_start_matches('_');
+            if canonical_trim == original_module.as_str().replace('.', "_") {
+                return true;
+            }
+        }


The logic for matching dotted-name private shims may produce false positives. It checks if the canonical module starts with '_' and the original contains '.', then compares by replacing dots with underscores. However, this could incorrectly match unrelated modules. For example, _foo_bar would match both foo.bar and foo_bar (a non-dotted module).

Consider adding more stringent checks, such as verifying that the canonical module is a top-level module (no dots in canonical_module), or using a more precise pattern matching approach to avoid false positives.

Copilot · 2025-12-29T08:15:10Z

pyrefly/lib/test/lsp/completion.rs

+#[test]
+fn autoimport_prefers_public_reexport_for_dotted_private_module() {
+    let code = r#"
+T = Thing
+#       ^
+"#;
+    let report = get_batched_lsp_operations_report_allow_error(
+        &[
+            ("main", code),
+            ("_foo_bar", "Thing = 1\n"),
+            ("foo.bar", "from _foo_bar import Thing\n"),
+        ],
+        get_test_report(Default::default(), ImportFormat::Absolute),
+    );
+    assert_eq!(
+        r#"
+# main.py
+2 | T = Thing
+            ^
+Completion Results:
+- (Variable) Thing: from foo.bar import Thing
+
+- (Variable) Thing: from _foo_bar import Thing
+
+
+
+# _foo_bar.py
+
+# foo.bar.py
+"#
+        .trim(),
+        report.trim(),
+    );
+}


The test validates the general pattern of preferring public re-exports over private modules for dotted module names (e.g., foo.bar vs _foo_bar), but it doesn't test the actual use case mentioned in issue #1685: collections.abc.Iterable vs _collections_abc.Iterable. Consider adding a test case that directly validates the stdlib pattern mentioned in the issue to ensure the fix works for the actual reported problem.

yangdanny97 · 2025-12-29T16:16:18Z

This might conflict with some in-flight changes from @javabster, so heads up that this might need to be rebased at some point in the next week or so

kinto0 · 2026-01-15T17:07:00Z

should this be reviewed (it's currently draft so we've been ignoring it)

asukaminato0721 · 2026-01-16T00:03:29Z

removed depth-based sorting in autoimport completions in lsp.rs, replacing it with a stable sort using label plus private rank to prefer public over private while keeping order consistent.

github-actions · 2026-01-16T00:13:35Z

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

meta-codesync · 2026-01-16T13:07:14Z

@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D90857539.

kinto0

thanks for working on this! this sort_text stuff is getting pretty confusing. I wonder if we can make a single function that sorts based on all the inputs (private module, length, etc) for autoimports, and a single function for completions (which calls the autoimports one for the import completions)? the logic being scattered across the file makes it difficult to understand

kinto0 · 2026-01-16T18:24:29Z

pyrefly/lib/state/lsp.rs

+                let is_private_import = handle_to_import_from
+                    .module()
+                    .components()
+                    .last()


i wonder if non-last components should also be considered?

I think that's what the referenced (#1685) issue is (_collections_abc.<>) so I'm not sure this PR fixes it without this change

kinto0 · 2026-01-16T18:42:24Z

pyrefly/lib/state/lsp.rs

                        None
                    },
-                    sort_text: Some(format!("4{}", depth)),
+                    sort_text: Some(format!("{name}:{private_rank}")),


this is pretty confusing and isn't obvious based on lsp trace why we are sorting them. could it make sense to include metadata in the sort text?

e.g.: "private: {private_rank}"

kinto0 · 2026-01-16T18:42:26Z

pyrefly/lib/state/lsp.rs

                    } else {
                        None
                    },
-                    sort_text: Some(format!("4{}", depth)),


why remove the 4?

kinto0 · 2026-01-16T18:46:08Z

pyrefly/lib/state/lsp.rs

                {
                    continue;
                }
-                let depth = handle_to_import_from.module().components().len();


we should also still consider depth. looks like CI didn't fail on this - would you be able to add a regression test that ensures shorter modules are preferred?

meta-cla bot added the cla signed label Dec 29, 2025

asukaminato0721 marked this pull request as ready for review December 29, 2025 08:11

Copilot AI review requested due to automatic review settings December 29, 2025 08:11

Copilot started reviewing on behalf of asukaminato0721 December 29, 2025 08:11 View session

Copilot AI reviewed Dec 29, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

asukaminato0721 force-pushed the 1685 branch from 84635d1 to 9a9fa38 Compare December 29, 2025 17:28

This comment has been minimized.

Sign in to view

kinto0 self-requested a review December 30, 2025 17:50

kinto0 self-assigned this Dec 30, 2025

kinto0 requested review from javabster and removed request for kinto0 December 30, 2025 17:52

kinto0 assigned javabster and unassigned kinto0 Dec 30, 2025

asukaminato0721 force-pushed the 1685 branch 3 times, most recently from 405e955 to caf186f Compare January 13, 2026 19:00

This comment has been minimized.

Sign in to view

asukaminato0721 marked this pull request as draft January 13, 2026 20:56

asukaminato0721 force-pushed the 1685 branch from c4383bc to cd22576 Compare January 15, 2026 06:19

This comment has been minimized.

Sign in to view

asukaminato0721 added 4 commits January 16, 2026 08:16

fix

8865dd2

.

aadaac7

fmt

4e53d24

.

2d89f6d

asukaminato0721 force-pushed the 1685 branch from cd22576 to 2d89f6d Compare January 15, 2026 23:16

This comment has been minimized.

Sign in to view

change sort method

436acdc

asukaminato0721 marked this pull request as ready for review January 16, 2026 00:03

kinto0 requested changes Jan 16, 2026

View reviewed changes

fix Auto import suggestions for collections.abc should take precedence over typing #1685 #1949

Are you sure you want to change the base?

fix Auto import suggestions for collections.abc should take precedence over typing #1685 #1949

Conversation

asukaminato0721 commented Dec 29, 2025

Summary

Test Plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

yangdanny97 commented Dec 29, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kinto0 commented Jan 15, 2026

Uh oh!

This comment has been minimized.

asukaminato0721 commented Jan 16, 2026

Uh oh!

github-actions bot commented Jan 16, 2026

Uh oh!

meta-codesync bot commented Jan 16, 2026

Uh oh!

kinto0 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinto0 Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

kinto0 Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinto0 Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinto0 Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

kinto0 Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kinto0 left a comment •

edited

Loading

kinto0 Jan 16, 2026 •

edited

Loading

kinto0 Jan 16, 2026 •

edited

Loading